Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

scripts/analyse_logs.py (2)

145-145: 💤 Low value

Consider renaming loop variable l to lvl or level.

The static analyzer flags l as ambiguous (could be confused with 1). This appears in several list comprehensions (lines 145, 166, 192, 226). While not a bug, more descriptive names improve readability.

♻️ Example fix for line 145

-        findings = [self.buckets[l].to_dict(l) for l in self.LEVELS if self.buckets[l].count]
+        findings = [self.buckets[lvl].to_dict(lvl) for lvl in self.LEVELS if self.buckets[lvl].count]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/analyse_logs.py` at line 145, Rename the ambiguous loop variable `l`
used in list comprehensions to a clearer name such as `level` (or `lvl`)
wherever it appears (e.g., in the comprehension that builds `findings =
[self.buckets[l].to_dict(l) for l in self.LEVELS if self.buckets[l].count]`),
update the variable inside the expression and its use when calling
`self.buckets[...].to_dict(...)`, and apply the same rename to the other similar
comprehensions that iterate over `self.LEVELS` (the occurrences that build other
lists and call `to_dict` on `self.buckets`) so the code uses `level`
consistently for readability.

---

319-319: Consider using itertools.pairwise() for successive pairs if Python 3.10+ is the minimum version.

itertools.pairwise() is more idiomatic than zip(minutes, minutes[1:]) for this pattern, but it requires Python 3.10+. If the minimum supported version is 3.10+, consider the refactoring below. Otherwise, the current zip() approach works correctly.

Suggested refactoring (Python 3.10+ only)

+from itertools import pairwise
 ...
-        for prev, curr in zip(minutes, minutes[1:]):
+        for prev, curr in pairwise(minutes):

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/analyse_logs.py` at line 319, Replace the manual sliding-window zip
pattern with itertools.pairwise when the project targets Python 3.10+: change
the loop that currently reads "for prev, curr in zip(minutes, minutes[1:]):" to
use itertools.pairwise(minutes) and add "from itertools import pairwise" at the
top of the module; keep the loop body unchanged so it still processes successive
(prev, curr) pairs from the minutes sequence.

scripts/analyse_logs.py (2)

272-274: 💤 Low value

Remove unnecessary int() cast.

round() already returns an integer in Python 3 when called with a single argument.

♻️ Suggested fix

         def pct(p: float) -> float:
-            i = max(0, min(n - 1, int(round(p / 100 * (n - 1)))))
+            i = max(0, min(n - 1, round(p / 100 * (n - 1))))
             return vs[i]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/analyse_logs.py` around lines 272 - 274, The pct function
unnecessarily casts round() to int; update pct(p: float) -> float to remove the
redundant int() call by using i = max(0, min(n - 1, round(p / 100 * (n - 1))))
and keep returning vs[i]; ensure you still clamp i between 0 and n-1 and that
variable names (pct, p, n, vs) remain unchanged.

---

323-323: 💤 Low value

Consider using itertools.pairwise() for successive pairs.

This is a cleaner pattern for iterating over consecutive elements. The strict= parameter isn't needed here since the slice guarantees correct pairing.

♻️ Suggested fix

Add import at the top:

from itertools import pairwise

Then update the loop:

-        for prev, curr in zip(minutes, minutes[1:]):
+        for prev, curr in pairwise(minutes):

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/analyse_logs.py` at line 323, Replace the manual sliding-window zip
loop for consecutive entries with itertools.pairwise: add "from itertools import
pairwise" at the top, then replace the "for prev, curr in zip(minutes,
minutes[1:]):" loop with "for prev, curr in pairwise(minutes):" to iterate
successive pairs of the minutes sequence (keep existing loop body unchanged).

scripts/analyse_logs.py (1)

315-336: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Heartbeat still misses minutes that had traffic but zero info logs.

This loop now detects only missing wall-clock minutes between observed timestamps. A minute with warn/error traffic and zero info lines is still omitted, so the probe doesn't actually cover the “info-level traffic dropped to zero mid-run” case from its own contract.

Suggested fix

-        minutes = sorted(set(self.minute_seen))
-        non_zero = [self.minute_info[m] for m in minutes if self.minute_info[m] > 0]
+        minutes = sorted(set(self.minute_seen))
+        non_zero = [self.minute_info.get(m, 0) for m in minutes if self.minute_info.get(m, 0) > 0]
         median = statistics.median(non_zero) if non_zero else 0
         gaps: list[dict] = []
-        # Walk consecutive observed minutes and flag any wall-clock minute that
-        # fell entirely between them with no log lines. Without this we'd never
-        # report a gap, since `minute_seen` only contains minutes that *had*
-        # traffic.
         if median >= 1 and len(minutes) >= 2:
+            for m in minutes[1:-1]:
+                if self.minute_info.get(m, 0) == 0 and len(gaps) < 200:
+                    gaps.append({"minute": m, "expected_min": int(median)})
             for prev, curr in pairwise(minutes):
                 try:
                     p = datetime.strptime(prev, "%Y-%m-%dT%H:%M")
                     c = datetime.strptime(curr, "%Y-%m-%dT%H:%M")
                 except ValueError:
                     continue
                 step = p + timedelta(minutes=1)
                 while step < c and len(gaps) < 200:
-                    gaps.append({
-                        "minute": step.strftime("%Y-%m-%dT%H:%M"),
-                        "expected_min": int(median),
-                    })
+                    minute = step.strftime("%Y-%m-%dT%H:%M")
+                    if all(g["minute"] != minute for g in gaps):
+                        gaps.append({"minute": minute, "expected_min": int(median)})
                     step += timedelta(minutes=1)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/analyse_logs.py` around lines 315 - 336, The code only flags
wall-clock minutes missing entirely between observed minutes but ignores minutes
that were observed in minute_seen yet have zero info-level lines (minute_info[m]
== 0); update the logic around minute_seen/minute_info to also treat those
zero-info observed minutes as gaps: after computing minutes and median, collect
zero_info = [m for m in minutes if self.minute_info.get(m, 0) == 0] and append
each as a gap entry (with "minute" and "expected_min": int(median)) while
keeping the existing between-minute walk (pairwise(minutes)) and the global cap
(len(gaps) < 200); ensure you deduplicate entries (e.g., via a set) so minutes
found by both methods aren’t added twice.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

.claude/commands/monitor.md (1)

43-53: ⚡ Quick win

Add a language tag to the fenced output-structure block.

Line 43 triggers MD040; adding a fence language keeps docs lint-clean.

💡 Suggested fix

-```
+```text
 logs/YYYYMMDD/HHMM_<slug>_<settings>/
 ├── <app>/raw/*.log          # cursor-filtered captures (one per iteration)
 ├── <app>/.cursor            # last-seen ISO timestamp per app
 ├── snapshots/
 │   ├── analysis_<HHMMSS>Z.md
 │   └── analysis_<HHMMSS>Z.json
 ├── analysis.md              # final probe report
 ├── analysis.json
 └── monitor.log              # verbose run history

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @.claude/commands/monitor.md around lines 43 - 53, The fenced code block in
.claude/commands/monitor.md (the output-structure block starting with
"logs/YYYYMMDD/HHMM__/") is missing a language tag which
triggers MD040; update the opening fence from totext so the block is
explicitly marked as plain text (no other edits to the block content needed).

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In @.claude/commands/monitor.md:

• Around line 43-53: The fenced code block in .claude/commands/monitor.md (the
  output-structure block starting with "logs/YYYYMMDD/HHMM__/") is
  missing a language tag which triggers MD040; update the opening fence from totext so the block is explicitly marked as plain text (no other edits to
  the block content needed).

</details>

---

<details>
<summary>ℹ️ Review info</summary>

<details>
<summary>⚙️ Run configuration</summary>

**Configuration used**: Path: .coderabbit.yaml

**Review profile**: CHILL

**Plan**: Pro

**Run ID**: `cb9e6ef6-8538-46ec-94dd-30215d66205f`

</details>

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 4d86dcc621b48417cfd10ed8b5f336febefb9ed6 and c0d25a90172e2504a7b2dfe92f603d2d48b5d0b3.

</details>

<details>
<summary>📒 Files selected for processing (4)</summary>

* `.claude/commands/monitor.md`
* `docs/development/DEVELOPMENT.md`
* `opencode.json`
* `scripts/logs.sh`

</details>

<details>
<summary>✅ Files skipped from review due to trivial changes (1)</summary>

* docs/development/DEVELOPMENT.md

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

scripts/logs.sh (2)

338-342: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use null-delimited cleanup for iteration JSONs.

This find output is marshalled through newline-delimited xargs rm -f, so spaces in OUTPUT_ROOT or an existing run path will split the filename and can skip or mis-target deletions.

Suggested fix

-                    iter_files=$(find "$run_dir" -type f -name '*_iter*.json' 2>/dev/null)
-                    if [[ -n "$iter_files" ]]; then
+                    if find "$run_dir" -type f -name '*_iter*.json' -print -quit 2>/dev/null | grep -q .; then
                         log_to_file "  Removing iteration JSONs: $date_dir/$run_name"
-                        printf '%s\n' "$iter_files" | xargs rm -f || \
+                        find "$run_dir" -type f -name '*_iter*.json' -print0 2>/dev/null | xargs -0 rm -f || \
                             log_to_file "  Failed to remove iteration JSONs in $run_dir"
                     fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/logs.sh` around lines 338 - 342, The current removal of iteration
JSONs collects filenames into iter_files and pipes them to xargs which breaks on
spaces; update the cleanup to use a null-delimited approach: change the find
invocation that sets iter_files (the code manipulating iter_files, run_dir and
the find command) to produce NUL-separated results (e.g., find ... -print0) and
invoke removal with a NUL-aware consumer (e.g., xargs -0 rm -f or find ... -exec
rm -f {} +), and keep the existing log_to_file calls (the messages that
reference date_dir/$run_name and the failure message) intact so failures still
log correctly.

---

187-194: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject empty app entries after splitting --app.

Line 191 only proves the first element is non-empty. Inputs like --app "hover,hover-worker," still create "$RUN_DIR//raw" and call flyctl logs --app "" on every iteration.

Suggested fix

     IFS=',' read -r -a APPS <<< "$APP"
     for i in "${!APPS[@]}"; do
         APPS[i]="${APPS[i]// /}"
     done
-    if [[ ${`#APPS`[@]} -eq 0 || -z "${APPS[0]}" ]]; then
+    if [[ ${`#APPS`[@]} -eq 0 ]]; then
         echo "at least one app name is required" >&2
         exit 1
     fi
+    for app in "${APPS[@]}"; do
+        if [[ -z "$app" ]]; then
+            echo "app list contains an empty value; check comma placement" >&2
+            exit 1
+        fi
+    done

Also applies to: 244-246

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/logs.sh` around lines 187 - 194, After splitting into APPS with
IFS=',' and trimming spaces in the existing for i in "${!APPS[@]}" loop, filter
out empty entries so blank elements (e.g., trailing commas) are removed before
further use; update the loop that currently does APPS[i]="${APPS[i]// /}" to
instead trim whitespace and discard any APPS[i] that becomes empty (rebuild a
new array of non-empty app names or compact APPS in-place), then update the
subsequent validation (the if [[ ${`#APPS`[@]} -eq 0 || -z "${APPS[0]}" ]]) to
check the filtered array; apply the same filtering fix to the identical
splitting/validation block around the later occurrence (the second APPS split at
lines ~244-246).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

scripts/logs.sh (4)

187-194: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate every parsed app name, not just APPS[0].

Inputs like --app "hover,hover-worker," still pass here, then create "$RUN_DIR//raw" and later call flyctl logs --app "". Reject empty entries after trimming, or rebuild APPS without them.

Suggested fix

     IFS=',' read -r -a APPS <<< "$APP"
     for i in "${!APPS[@]}"; do
         APPS[i]="${APPS[i]// /}"
     done
-    if [[ ${`#APPS`[@]} -eq 0 || -z "${APPS[0]}" ]]; then
+    if [[ ${`#APPS`[@]} -eq 0 ]]; then
         echo "at least one app name is required" >&2
         exit 1
     fi
+    for app_name in "${APPS[@]}"; do
+        if [[ -z "$app_name" ]]; then
+            echo "app list contains an empty value; check comma placement" >&2
+            exit 1
+        fi
+    done

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/logs.sh` around lines 187 - 194, The current validation only checks
APPS[0], allowing empty entries like trailing commas; after splitting into APPS
(IFS=',' read -r -a APPS) and trimming (the loop that does APPS[i]="${APPS[i]//
/}"), filter out any empty strings by rebuilding APPS to include only non-empty
elements (e.g., iterate over APPS and append only if not empty) and then perform
the required emptiness check on the rebuilt APPS array so inputs like
"hover,hover-worker," are rejected and no empty app names are used later.

---

338-342: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use a null-delimited cleanup pipeline here.

This find → plain xargs rm -f path splits on whitespace, so runs with spaces in their path can leave iteration JSON behind or hand broken tokens to rm.

Suggested fix

-                    iter_files=$(find "$run_dir" -type f -name '*_iter*.json' 2>/dev/null)
-                    if [[ -n "$iter_files" ]]; then
+                    if find "$run_dir" -type f -name '*_iter*.json' -print -quit 2>/dev/null | grep -q .; then
                         log_to_file "  Removing iteration JSONs: $date_dir/$run_name"
-                        printf '%s\n' "$iter_files" | xargs rm -f || \
+                        find "$run_dir" -type f -name '*_iter*.json' -print0 2>/dev/null | xargs -0 rm -f || \
                             log_to_file "  Failed to remove iteration JSONs in $run_dir"
                     fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/logs.sh` around lines 338 - 342, The cleanup pipeline fails on paths
with whitespace because iter_files is fed to plain xargs; change the logic
around the iter_files check to use a null-delimited find output (e.g., replace
collecting into iter_files and printf ... | xargs rm -f with a null-safe
pipeline using find ... -print0 piped to xargs -0 rm -f, or iterate with while
IFS= read -r -d '' file; do rm -f "$file"; done < <(find "$run_dir" -type f
-name '*_iter*.json' -print0)); keep the existing log_to_file invocations (e.g.,
the "Removing iteration JSONs: $date_dir/$run_name" and failure log) and ensure
you still only run the removal when matching files exist.

---

230-240: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Sanitise --run-id before composing RUN_DIR.

RUN_ID is interpolated straight into a filesystem path. Values containing /, \, or .. can escape the advertised logs/YYYYMMDD/HHMM_<slug>_<settings>/ layout and confuse the later cleanup logic that assumes runs sit exactly two levels below "$OUTPUT_ROOT".

Suggested fix

+    if [[ -n "$RUN_ID" && ! "$RUN_ID" =~ ^[A-Za-z0-9._-]+$ ]]; then
+        echo "run-id may only contain letters, numbers, dot, underscore, and hyphen" >&2
+        exit 1
+    fi
+
     if [[ -z "$RUN_ID" ]]; then
         RUN_ID="$(generate_run_slug)_${SETTINGS_SUFFIX}"
     else
         RUN_ID="${RUN_ID}_${SETTINGS_SUFFIX}"
     fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/logs.sh` around lines 230 - 240, Sanitise RUN_ID before composing
RUN_DIR: ensure the value produced by generate_run_slug or supplied via --run-id
is stripped of path traversal characters and separators (like '/' and '\' and
any '..' segments) and normalized to a safe slug (e.g., remove/replace those
characters and collapse repeated dots) so it cannot escape OUTPUT_ROOT; then use
that sanitized variable when building RUN_DIR (referencing RUN_ID and RUN_DIR in
the snippet) and ensure it never becomes empty (fallback to a safe default if
needed).

---

30-43: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Require a real Python 3 probe in both interpreter selectors.

python -c "import sys" also succeeds on Python 2, so search, analyse, and the monitor-side helpers can still bind to an unsupported interpreter and fail later. Please gate both blocks on sys.version_info[0] >= 3, ideally via one shared probe so they cannot drift again.

Suggested fix

+check_python3() {
+    "$@" - <<'PY' >/dev/null 2>&1
+import sys
+raise SystemExit(0 if sys.version_info[0] >= 3 else 1)
+PY
+}
+
 resolve_python() {
-    if command -v python3 >/dev/null 2>&1 && python3 -c "import sys" >/dev/null 2>&1; then
+    if command -v python3 >/dev/null 2>&1 && check_python3 python3; then
         PYTHON_CMD="python3"
         PYTHON_ARGS=()
-    elif command -v python >/dev/null 2>&1 && python -c "import sys" >/dev/null 2>&1; then
+    elif command -v python >/dev/null 2>&1 && check_python3 python; then
         PYTHON_CMD="python"
         PYTHON_ARGS=()
-    elif command -v py >/dev/null 2>&1 && py -3 -c "import sys" >/dev/null 2>&1; then
+    elif command -v py >/dev/null 2>&1 && check_python3 py -3; then
         PYTHON_CMD="py"
         PYTHON_ARGS=(-3)
     else
         echo "Python 3 is required for this command but was not found." >&2
         exit 1
@@
-    if command -v python3 >/dev/null 2>&1 && python3 -c "import sys" >/dev/null 2>&1; then
+    if command -v python3 >/dev/null 2>&1 && check_python3 python3; then
         PYTHON_CMD="python3"
-    elif command -v python >/dev/null 2>&1 && python -c "import sys" >/dev/null 2>&1; then
+    elif command -v python >/dev/null 2>&1 && check_python3 python; then
         PYTHON_CMD="python"
-    elif command -v py >/dev/null 2>&1 && py -3 -c "import sys" >/dev/null 2>&1; then
+    elif command -v py >/dev/null 2>&1 && check_python3 py -3; then
         PYTHON_CMD="py"
         PYTHON_ARGS=(-3)
     fi

Also applies to: 196-203

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/logs.sh` around lines 30 - 43, In resolve_python(), the current
probes (python3 -c "import sys" and python -c "import sys") can succeed on
Python 2; change both checks to validate major version >=3 (e.g. replace the
probe commands with python3 -c 'import sys; sys.exit(0 if sys.version_info[0] >=
3 else 1)' and python -c 'import sys; sys.exit(0 if sys.version_info[0] >= 3
else 1)') so PYTHON_CMD/PYTHON_ARGS only bind to a true Python 3 interpreter;
alternatively factor this into a single shared probe helper used by the python3
and python branches (referencing resolve_python, PYTHON_CMD, PYTHON_ARGS) and
apply the same pattern for the py branch as needed.

scripts/analyse_logs.py (1)

20-20: 💤 Low value

Unused import: zipfile.

The zipfile module is imported but never used in this file — iter_lines() from search_logs.py already handles .zip extraction internally.

♻️ Suggested fix

-import zipfile

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/analyse_logs.py` at line 20, Remove the unused import of the zipfile
module from scripts/analyse_logs.py; since zip extraction is already handled by
iter_lines() in search_logs.py, delete the line importing zipfile to avoid an
unused import warning and keep imports minimal.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

scripts/analyse_logs.py (1)

241-248: ⚠️ Potential issue | 🟠 Major

Keep latency percentile state bounded.

self.values_ms still retains one float per latency sample for the full app analysis. Because this script is rerun against the growing run for periodic snapshots, high-volume runs can still fail from memory growth even though self.slowest is trimmed. Please switch percentile calculation to a bounded histogram/sketch instead of materializing every sample.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/analyse_logs.py` around lines 241 - 248, The code keeps every latency
sample in self.values_ms which causes unbounded memory growth; change the
implementation so _record no longer appends raw floats but instead updates a
bounded histogram/sketch data structure (e.g., fixed-size bucket histogram,
t-digest, or a reservoir/two-phase sketch) and maintain total_count and bucket
counts; update __init__ to initialize the chosen structure (replace
self.values_ms: list[float] with something like self._histogram/buckets and
self._total_count) and modify _record(ms, ts, line) to validate ms, increment
the appropriate bucket and possibly update self.slowest, and add a percentile()
helper that computes percentiles by walking buckets (or querying the sketch);
ensure no code path retains per-sample floats so memory usage stays bounded
while percentiles remain computable.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

scripts/analyse_logs.py (1)

241-304: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

LatencyProbe still grows memory unbounded with traffic volume.

At Line [242] and Line [248], every latency sample is retained for the full analysis window. On long/high-volume runs this can exhaust memory and break periodic/final analysis. Please switch percentile tracking to a bounded streaming structure (histogram/sketch/reservoir) and keep only bounded top-N slowest lines.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/analyse_logs.py` around lines 241 - 304, The LatencyProbe currently
stores every sample in self.values_ms causing unbounded memory growth; replace
the unbounded list with a bounded streaming quantile structure (e.g. a
fixed-size reservoir sampler or a quantile sketch like t-digest) and leave
self.slowest as the bounded top-N list. Concretely: in __init__ replace
self.values_ms: list[float] with a bounded reservoir/sketch instance; update
_record to insert into that reservoir/sketch instead of appending to
self.values_ms (still validate ms range and keep self.slowest/top-N trimming
logic unchanged); update report to compute p50/p95/p99/mean/max from the
reservoir/sketch’s APIs (or, for reservoir, compute percentiles from the
reservoir snapshot) and handle empty-case as before. Use the existing method
names (__init__, _record, feed, report) and fields (self.values_ms -> new
reservoir/sketch, self.slowest) so callers remain compatible.

scripts/analyse_logs.py (1)

418-421: ⚡ Quick win

Handle invalid --regex input as a user error instead of a traceback.

User-supplied regex is compiled directly; invalid patterns currently raise re.error and crash noisily. Add validation with a clear stderr message and exit code 2 (arg error behavior).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/analyse_logs.py` around lines 418 - 421, Validate user-provided
regexes in extra_regexes before adding them to patterns: for each r in
extra_regexes attempt re.compile(r) in a try/except block, and if re.error is
raised write a clear error message to stderr (including the offending pattern)
and exit with code 2 (sys.exit(2)); if compilation succeeds continue to append
(f"regex:{r}", r) to patterns as before so RegexBucketProbe("Ad-hoc keywords",
patterns) receives the same items but invalid regex inputs are handled as a user
error.

scripts/analyse_logs.py (1)

241-249: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Bound latency sample storage before snapshot analysis scales up.

self.values_ms still retains one float per latency event for the full app scan. Because logs.sh monitor re-runs analysis over the growing run every few minutes, a busy app can still push snapshot/final analysis into avoidable memory failure.

Also applies to: 270-304

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/analyse_logs.py` around lines 241 - 249, The code currently appends
every latency into self.values_ms and self.slowest, causing unbounded memory
growth; modify __init__ and _record to keep bounded state: introduce constants
like SAMPLE_SIZE (e.g. 10000) and SLOWEST_KEEP (e.g. 100) and implement
reservoir sampling for self.values_ms to maintain a fixed-size random sample
instead of appending every ms, and maintain self.slowest as a fixed-size
min-heap (use heapq) that pushes (ms, ts, line) and pops the smallest when
length > SLOWEST_KEEP so only the top-N slowest are retained; apply the same
bounded-sampling logic to the other recording code paths referenced (the code
around the other recording block) so all latency collectors use the same
SAMPLE_SIZE and SLOWEST_KEEP behavior.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

scripts/analyse_logs.py (1)

241-304: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Bound latency sample storage to prevent analysis memory blow-ups.

LatencyProbe still retains every latency in self.values_ms, which can grow without bound on long/high-volume runs and fail periodic/final analysis. Please switch percentile calculation to a bounded streaming structure (for example, histogram/t-digest/reservoir) and keep slowest entries in a fixed-size top-N heap.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/analyse_logs.py` around lines 241 - 304, The LatencyProbe currently
appends every sample to self.values_ms and self.slowest; change this to bounded
structures: replace unbounded self.values_ms with a fixed-size reservoir (e.g.
reservoir_size constant, maintain a reservoir list and a counter and use random
replacement in _record) so percentiles in report() are computed from the
reservoir instead of all samples, and replace self.slowest list logic with a
fixed-size min-heap (use heapq) kept in _record to maintain the top-N slowest
entries (push then pop when size exceeds max_slowest); update _record, __init__,
and report to use the new reservoir and heap (compute sorted percentiles from
the reservoir, and produce slowest by popping the heap into descending order).